fix(storage): allow for Age int64* type and int64 type #6230

frankyn · 2022-06-21T21:32:04Z

I added a helper func to handle different types (int64 and int64*) when reading data from Storage Apiary client.
I didn't add from Manual -> Apiary client because I need some help typecasting without Go yelling at me.

Towards unblocking: #6204

…loud-go into reflect-condition-field

tritone

Very nice, thanks for adding this Frank!

tritone · 2022-06-22T15:17:08Z

storage/bucket.go

+	v := reflect.ValueOf(cond).Elem().FieldByName("Age")
+	if v.Kind() == reflect.Int64 {
+		return v.Interface().(int64)
+	} else if v.Kind() == reflect.Pointer {


Pointer got added in a recent Go release; use reflect.Ptr here instead. (This is why the CI fails.)

tritone · 2022-06-22T15:19:45Z

storage/bucket.go

@@ -1503,6 +1503,17 @@ func toCORSFromProto(rc []*storagepb.Bucket_Cors) []CORS {
 	return out
 }

+// Handle ptr or int64
+func setAgeCondition(age int64, cond interface{}) {


Add a note about why this is needed and a TODO/issue to remove in a future release.

codyoss · 2022-06-22T16:16:17Z

storage/bucket.go

@@ -1525,6 +1536,8 @@ func toRawLifecycle(l Lifecycle) *raw.BucketLifecycle {
 			},
 		}

+		setAgeCondition(r.Condition.AgeInDays, rr.Condition)


Suggested change

setAgeCondition(r.Condition.AgeInDays, rr.Condition)

setAgeCondition(r.Condition.AgeInDays, &rr.Condition)

codyoss · 2022-06-22T16:19:03Z

storage/bucket_test.go

+	var tp testPointer
+	var want int64 = 10
+	setAgeCondition(want, &tp)
+	if getAgeCondition(&tp) != want {


Suggested change

if getAgeCondition(&tp) != want {

if getAgeCondition(tp) != want {

codyoss · 2022-06-22T16:19:41Z

storage/bucket_test.go

+	var ti testInt64
+	want = 100
+	setAgeCondition(100, &ti)
+	if getAgeCondition(&ti) != want {


Suggested change

if getAgeCondition(&ti) != want {

if getAgeCondition(ti) != want {

codyoss · 2022-06-22T16:20:05Z

storage/bucket_test.go

+	var want int64 = 10
+	setAgeCondition(want, &tp)
+	if getAgeCondition(&tp) != want {
+		t.Fatalf("got %v, want 10", *tp.Age)


Suggested change

t.Fatalf("got %v, want 10", *tp.Age)

t.Fatalf("got %v, want %v", *tp.Age, want)

codyoss · 2022-06-22T16:32:39Z

storage/bucket.go

@@ -1503,6 +1503,17 @@ func toCORSFromProto(rc []*storagepb.Bucket_Cors) []CORS {
 	return out
 }

+// Handle ptr or int64
+func setAgeCondition(age int64, cond interface{}) {
+	c := reflect.ValueOf(cond).Elem().FieldByName("Age")


I would limit the amount of reflection needed by just passing in a pointer to the field directly -- do a recursive call if needed to go one more level in.

codyoss · 2022-06-22T16:33:23Z

storage/bucket.go

@@ -1595,6 +1608,19 @@ func toProtoLifecycle(l Lifecycle) *storagepb.Bucket_Lifecycle {
 	return &rl
 }

+// Handle ptr or int64
+func getAgeCondition(cond interface{}) int64 {
+	v := reflect.ValueOf(cond).Elem().FieldByName("Age")


Same for this one, I would just pass in the field directly.

codyoss · 2022-06-22T16:34:38Z

Some of my comments might not make sense if you refactor the getter/setter into working directly on fields FYI. Feel free to dismiss those

codyoss · 2022-06-22T16:35:20Z

Also forgot to ask, how much of a hot path is this code on for normal operations? There is be a penalty for temporarily using reflection.

frankyn · 2022-06-22T17:38:19Z

Also forgot to ask, how much of a hot path is this code on for normal operations? There is be a penalty for temporarily using reflection.

Anytime a customer gets Bucket.Attrs(), it will go through this path iff OLM policies are set on the bucket.
It will be a common path for customers but not as common as working with uploading or downloading objects.

storage/bucket.go

codyoss

LGTM, thanks for all the changes!

Towards unblocking: googleapis/google-cloud-go#6230 Change Storage OLM Age condition type from int64 to *int64 to allow for existence of check Co-authored-by: Chris Cotter <cjcotter@google.com>

Albert-Gao · 2022-09-04T23:11:44Z

interesting, this PR "introduced" a breaking change... which breaks firebase.google.com/go/v4 v4.8.0
but I do not know why Golang installed the 1.22.1 of the this lib rather than 1.21 that firebase-admin-go depends on :D

tritone · 2022-09-06T00:42:14Z

interesting, this PR "introduced" a breaking change... which breaks firebase.google.com/go/v4 v4.8.0 but I do not know why Golang installed the 1.22.1 of the this lib rather than 1.21 that firebase-admin-go depends on :D

@Albert-Gao I believe the error you are seeing is from having incompatible versions of cloud.google.com/go/storage and google.golang.org/api. Also, I think it is an issue with your project's build rather than with firebase.google.com/go/v4 because the deps for the firebase package predate this change.

I would try upgrading both cloud.google.com/go/storage and google.golang.org/api to the most recent version, or downgrading google.golang.org/api to <= 0.93.0 if you need to stay on an older version of storage.

If this doesn't work, please file an issue rather than adding a PR comment here.

Albert-Gao · 2022-09-06T10:31:38Z

thanks, @tritone :)

product-auto-label bot added the size: s Pull request size is small. label Jun 21, 2022

frankyn force-pushed the reflect-condition-field branch from a063185 to 5df45dc Compare June 22, 2022 00:01

fix: allow for pointer based types

8a9137c

frankyn force-pushed the reflect-condition-field branch from 5df45dc to 8a9137c Compare June 22, 2022 00:58

Merge branch 'main' into reflect-condition-field

fdca0f4

frankyn marked this pull request as ready for review June 22, 2022 00:59

frankyn requested review from a team as code owners June 22, 2022 00:59

frankyn changed the title ~~fix: allow for pointer based types~~ fix(storage): allow for Age int64* type and int64 type Jun 22, 2022

frankyn added 2 commits June 21, 2022 21:16

add tests

52def10

Merge branch 'reflect-condition-field' of github.com:frankyn/google-c…

edc56f4

…loud-go into reflect-condition-field

product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jun 22, 2022

tritone reviewed Jun 22, 2022

View reviewed changes

codyoss reviewed Jun 22, 2022

View reviewed changes

frankyn added 2 commits June 22, 2022 09:40

Pointer -> Ptr and update test constant to use want

42c8307

address feedback

074312d

product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Jun 22, 2022

remove AgeInDays set by autogen client

df7ace9

product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jun 22, 2022

frankyn mentioned this pull request Jun 22, 2022

storage: remove OLM Age condition reflection #6240

Closed

address todo feedback

a0b26ba

frankyn requested review from tritone and codyoss June 22, 2022 18:47

codyoss reviewed Jun 22, 2022

View reviewed changes

storage/bucket.go Outdated Show resolved Hide resolved

storage/bucket.go Outdated Show resolved Hide resolved

storage/bucket.go Outdated Show resolved Hide resolved

address feedback

f49b7f4

frankyn requested a review from codyoss June 22, 2022 19:22

Merge branch 'main' into reflect-condition-field

fa7c02f

codyoss approved these changes Jun 22, 2022

View reviewed changes

frankyn mentioned this pull request Jun 22, 2022

fix(storage): *int64 instead of int64 for Age cond googleapis/google-api-go-client#1598

Merged

tritone approved these changes Jun 22, 2022

View reviewed changes

frankyn mentioned this pull request Jun 22, 2022

fix(storage): allow to use age=0 in OLM conditions #6204

Merged

frankyn merged commit cc7acb8 into googleapis:main Jun 22, 2022

frankyn deleted the reflect-condition-field branch June 22, 2022 20:01

release-please bot mentioned this pull request Jun 23, 2022

chore(main): release storage 1.23.0 #6140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(storage): allow for Age int64* type and int64 type #6230

fix(storage): allow for Age int64* type and int64 type #6230

frankyn commented Jun 21, 2022 •

edited

Loading

tritone left a comment

tritone Jun 22, 2022

tritone Jun 22, 2022

codyoss Jun 22, 2022

codyoss Jun 22, 2022

codyoss Jun 22, 2022

codyoss Jun 22, 2022

codyoss Jun 22, 2022

codyoss Jun 22, 2022

codyoss commented Jun 22, 2022

codyoss commented Jun 22, 2022

frankyn commented Jun 22, 2022

codyoss left a comment

Albert-Gao commented Sep 4, 2022 •

edited

Loading

tritone commented Sep 6, 2022 •

edited

Loading

Albert-Gao commented Sep 6, 2022

	setAgeCondition(r.Condition.AgeInDays, rr.Condition)
	setAgeCondition(r.Condition.AgeInDays, &rr.Condition)

	if getAgeCondition(&tp) != want {
	if getAgeCondition(tp) != want {

	if getAgeCondition(&ti) != want {
	if getAgeCondition(ti) != want {

	t.Fatalf("got %v, want 10", *tp.Age)
	t.Fatalf("got %v, want %v", *tp.Age, want)

fix(storage): allow for Age int64* type and int64 type #6230

fix(storage): allow for Age int64* type and int64 type #6230

Conversation

frankyn commented Jun 21, 2022 • edited Loading

tritone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codyoss commented Jun 22, 2022

codyoss commented Jun 22, 2022

frankyn commented Jun 22, 2022

codyoss left a comment

Choose a reason for hiding this comment

Albert-Gao commented Sep 4, 2022 • edited Loading

tritone commented Sep 6, 2022 • edited Loading

Albert-Gao commented Sep 6, 2022

frankyn commented Jun 21, 2022 •

edited

Loading

Albert-Gao commented Sep 4, 2022 •

edited

Loading

tritone commented Sep 6, 2022 •

edited

Loading